Skip to content
This repository has been archived by the owner on Aug 14, 2020. It is now read-only.

discovery: return only best matching templates. #596

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sgotti
Copy link
Member

@sgotti sgotti commented Apr 12, 2016

Actually the spec doesn't clarify when an endpoint template should be accepted or
not. Now, the appc/spec implementation returns only endpoints that can
be fully rendered. This means that it will also accept a template if it
doesn't contain some of the required discovery labels.

This looks good, but, trying to implement some meta discovery ideas it
will bring to unwanted endpoints.

Example 1:
Suppose I want to implement the "latest" pattern behavior:

<!-- ACIs with specific version -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-{version}-{os}-{arch}.{ext}">
<!-- Latest ACIs -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-latest-{os}-{arch}.{ext}">

If, on discovery, I ask for the name, os and arch
labels only the second template will be rendered (since the first cannot
be fully rendered due to missing version label). So I achieved latest
pattern.

But if I'm asking for a specific version both templates will be
rendered.

Example 2:
As an extension of the first example, suppose I want to create a global
meta discovery for all my images on example.com. So on the root of my
server I'll put some meta tags using example.com as prefix:

<!-- ACIs with specific version -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-{version}-{os}-{arch}.{ext}">
<meta name="ac-discovery" content="example.com https://mirror.storage.example.com/{name}-{version}-{os}-{arch}.{ext}">
<!-- noarch ACIs -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-{version}-noarch.{ext}">
<meta name="ac-discovery" content="example.com https://mirror.storage.example.com/{name}-{version}-noarch.{ext}">
<!-- Latest ACIs -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-latest-{os}-{arch}.{ext}">
<meta name="ac-discovery" content="example.com https://mirror.storage.example.com/{name}-latest-{os}-{arch}.{ext}">
<!-- Latest noarch ACIs -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-latest-noarch.{ext}">
<meta name="ac-discovery" content="example.com https://mirror.storage.example.com/{name}-latest-noarch.{ext}">

With this tags I want to implement global "latest" and "noarch" patterns
and also return multiple mirrors.

If, on discovery, I ask only for the name and version labels the
template 3 and 4 will be rendered. So I achieved a versioned noarch pattern.

But, unfortunately, also the last two templates will be rendered.

And, as the first example, if I'm asking for a specific name,
version, os and arch ALL the templates will be rendered.

Since the spec says:

Note that multiple `ac-discovery` tags MAY be returned for a given
prefix-match [snip] In this case, the client implementation MAY choose
which to use at its own discretion.

If an implementation chooses the second it can download the wrong image version.

This patch makes template matching stricter choosing only best matching templates.
Best matching templates are the one where all the templates labels can
be substituted and with the highest number of substituted labels.

With this we can obtain various patterns like latest and noarch and also
keeping the ability to return multiple mirror urls. See also the tests
added in this commit.

It also documents this behavior.

It should not BREAK many of the current meta tags implementation (it
depends on how the client uses the returned endpoints, for example rkt,
currently, only uses the first one)

This is also needed to implement #575 second proposal (remove the "hidden" behavior that sets a version label to "latest" if not provided)

@sgotti sgotti force-pushed the discovery_return_only_best_matching_templates branch 2 times, most recently from 5b7134f to 8d0262c Compare April 12, 2016 10:24
&mockHTTPDoer{
doer: fakeHTTPGet(
[]meta{
{"/myapp",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formatting is a little weird (elsewhere too) - newline before " ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Fixed.

@jonboulle
Copy link
Contributor

Great write-up 👏. I mostly agree with your diagnosis and suggestion, just added a few comments on the details.

Actually the spec doesn't clarify when an endpoint template should be accepted or
not. Now, the appc/spec implementation returns only endpoints that can
be fully rendered. This means that it will also accept a template if it
doesn't contain some of the required discovery labels.

This looks good, but, trying to implement some meta discovery ideas it
will bring to unwanted endpoints.

Example 1:
Suppose I want to implement the "latest" pattern behavior:

```html
<!-- ACIs with specific version -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-{version}-{os}-{arch}.{ext}">
<!-- Latest ACIs -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-latest-{os}-{arch}.{ext}">
```

If, on discovery, I ask for the _name_, _os_ and _arch_
labels only the second template will be rendered (since the first cannot
be fully rendered due to missing _version_ label). So I achieved latest
pattern.

But if I'm asking for a specific _version_ both templates will be
rendered.

Example 2:
As an extension of the first example, suppose I want to create a global
meta discovery for all my images on example.com. So on the root of my
server I'll put some meta tags using example.com as prefix:

```html
<!-- ACIs with specific version -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-{version}-{os}-{arch}.{ext}">
<meta name="ac-discovery" content="example.com https://mirror.storage.example.com/{name}-{version}-{os}-{arch}.{ext}">
<!-- noarch ACIs -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-{version}-noarch.{ext}">
<meta name="ac-discovery" content="example.com https://mirror.storage.example.com/{name}-{version}-noarch.{ext}">
<!-- Latest ACIs -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-latest-{os}-{arch}.{ext}">
<meta name="ac-discovery" content="example.com https://mirror.storage.example.com/{name}-latest-{os}-{arch}.{ext}">
<!-- Latest noarch ACIs -->
<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-latest-noarch.{ext}">
<meta name="ac-discovery" content="example.com https://mirror.storage.example.com/{name}-latest-noarch.{ext}">
```

With this tags I want to implement global "latest" and "noarch" patterns
and also return multiple mirrors.

If, on discovery, I ask only for the _name_ and _version_ labels the
template 3 and 4 will be rendered. So I achieved a versioned noarch pattern.

But, unfortunately, also the last two templates will be rendered.

And, as the first example, if I'm asking for a specific _name_,
_version_, _os_ and _arch_ ALL the templates will be rendered.

Since the spec says:
```
Note that multiple `ac-discovery` tags MAY be returned for a given
prefix-match [snip] In this case, the client implementation MAY choose
which to use at its own discretion.
```

If an implementation chooses the second it can download the wrong image version.

This patch makes template matching stricter choosing only best matching templates.
Best matching templates are the one where all the templates labels can
be substituted and with the highest number of substituted labels.

With this we can obtain various patterns like latest and noarch and also
keeping the ability to return multiple mirror urls. See also the tests
added in this commit.

It also documents this behavior.

It should not BREAK many of the current meta tags implementation (it
depends on how the client uses the returned endpoints, for example rkt,
currently, only uses the first one)
@sgotti sgotti force-pushed the discovery_return_only_best_matching_templates branch from 8d0262c to d5f8a46 Compare April 12, 2016 14:38
@sgotti
Copy link
Member Author

sgotti commented Apr 13, 2016

@jonboulle Thanks.

I'm on the fence between:

IMHO the former is stricter and better (since it'll fail discovery if a template doesn't contain some client provided labels) but I've the fear that it'll break some current implementations (perhaps this is a pointless fear).

I'm open to change it back to #580 behavior (changing this PR since it provides more tests) if you are ok.

@tdavis
Copy link

tdavis commented May 27, 2016

@sgotti If we were to expand the meta tag format slightly, I think it would be possible to maintain flexibility without introducing the ambiguity that comes from not requiring full substitution. Consider:

<meta name="ac-discovery" content="example.com https://storage.example.com/{name}/{version}/{arch}/{name}.{ext}" arch="noarch" version="latest">

We could supply optional default values for missing variables. This allows for #580 (which I agree is better than a "best match" which could easily result in invalid URLs) while also avoiding the need to maintain many different tags to cover all the permutations.

Additionally, while I agree in principle that all client supplied labels should be substituted, there's no way to know from the tag what those variables are (without implementing the RFC and parsing the URL). Another useful extension would be:

<meta ... requires="foo,bar,baz"> 

Where the required fields would just be ones present and without default values. Taken together, here's Example 2 above:

<meta name="ac-discovery" content="example.com https://storage.example.com/{name}-{version}-{os}-{arch}.{ext}" version="latest" os="any" requires="name">
<meta name="ac-discovery" content="example.com https://mirror.example.com/{name}-{version}-{os}-{arch}.{ext}" version="latest" os="any" requires="name">

This reduces the eight required tags to two, plus gives you OS-independent permutations. It also allows the client to quickly find usable URLs just by checking the requires attribute against available labels. This also shouldn't affect existing implementations at all. Thoughts?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants